Overall looks good. I had one minor comment (a duplicate line of code I think) and we could use some docs (at a minimum just add the description of this PR to an .md doc somewhere)

I'm going to check out the code and do some testing before I give the PR an official approval.

I'm back and going to address the documentation in an additional ticket #2875

Code looks good overall, I flagged a couple items that need to be looked at. Also wondering if it would make sense (and there is a way) to test a failed upgrade and how to recover from that?

I'm not sure of how I can force an upgrade to fail with the integration tests. I can look into making sure it's covered by unit tests.

My idea is to suggest using the wallet-type which you can get via the api to handle both endpoints when doing an upgrade. In that case if it fails then it will continue working as before. If it succeeds the controller can remove the old endpoints in a future release. This is basically how the integration test controller is implemented now. I think it will be clear once I finish the upgrade documentation.

Will address the other comments and make sure upgrade fail is covered by some type of test.

Code looks good overall, I flagged a couple items that need to be looked at. Also wondering if it would make sense (and there is a way) to test a failed upgrade and how to recover from that?

I'm not sure of how I can force an upgrade to fail with the integration tests. I can look into making sure it's covered by unit tests.

My idea is to suggest using the wallet-type which you can get via the api to handle both endpoints when doing an upgrade. In that case if it fails then it will continue working as before. If it succeeds the controller can remove the old endpoints in a future release. This is basically how the integration test controller is implemented now. I think it will be clear once I finish the upgrade documentation.

Will address the other comments and make sure upgrade fail is covered by some type of test.

Depending on effort, if it doesn't too much coordinating to handle a fail it would be useful. I was wondering situations such as corrupted records that would cause trouble (is it even possible?) or the service being terminated mid-way, specifically. likely easier to deal with in unit test than integration tests, I think 😅

• Added a middleware that checks the singleton and blocks traffic for the particular wallet if upgrade is in progress.

Will this work in a scaled environment? I.e. should a multi-instance environment scale down to a single instance before upgrading wallets?

I don't think the upgrade should take very long for any agents or subwallets as the DB changes are just removing and replacing records.

... as long as we're not updating records for individual credentials (OrgBook has millions of credentials in it's wallet)

As I understand it, the plan is that the controller is responsible for scaling down, then start back up and calling this endpoint during controller initialization before proceeding. So the ACA-Py instance doesn’t have to scale down as that is impossible to manage. But the controller does.

As I understand it, the plan is that the controller is responsible for scaling down, then start back up and calling this endpoint during controller initialization before proceeding. So the ACA-Py instance doesn’t have to scale down as that is impossible to manage. But the controller does.

We need a document :-P (I didn't notice when I approved the PR but went back and checked after @swcurran 's comment) Doesn't need to be part of this PR but we will def need one.

I don't think the upgrade should take very long for any agents or subwallets as the DB changes are just removing and replacing records.

... as long as we're not updating records for individual credentials (OrgBook has millions of credentials in it's wallet)

Only the schema, cred_def and revocation objects are getting updated. The revocation objects could potentially take some time. I don't have much experience with this in heavy loads. The individual credentials aren't effected.

As I understand it, the plan is that the controller is responsible for scaling down, then start back up and calling this endpoint during controller initialization before proceeding. So the ACA-Py instance doesn’t have to scale down as that is impossible to manage. But the controller does.

We need a document :-P (I didn't notice when I approved the PR but went back and checked after @swcurran 's comment) Doesn't need to be part of this PR but we will def need one.

Yes. I'm still working on a detailed document with the upgrade process and the migration steps via #2875. Should be ready soon.

@ianco I'm going to create a separate document for the design of this with some diagrams so it's clear and can be referenced in the future. It will be separate from the migration guide.

@ianco @swcurran I'm removing the in memory singleton part of this, and instead using the wallet(DB) value in the middleware. I hadn't considered the scaled up, multi instance use case correctly.

I've already made the changes and am testing. The down side is there will be a small performance hit I wanted to avoid.

I wanted to add a DB hook to maintain the in memory singleton mechanism between instances. But after researching it I can't see this done anywhere in the project and it looks like it would be a substantial effort to implement.

@ianco @swcurran I'm removing the in memory singleton part of this, and instead using the wallet(DB) value in the middleware. I hadn't considered the scaled up, multi instance use case correctly.

I've already made the changes and am testing. The down side is there will be a small performance hit I wanted to avoid.

I wanted to add a DB hook to maintain the in memory singleton mechanism between instances. But after researching it I can't see this done anywhere in the project and it looks like it would be a substantial effort to implement.

This update looks good, but I'm wondering also about the performance hit. There is a possibility of using a redis cache (https://github.com/Indicio-tech/aries-acapy-cache-redis - not sure if this one has made it to the plug=ins repo) to keep the state in memory across instances (of course in that case aca-py would have to be configured with that plug-in).

Another option is to add an aca-py flag as to whether to do the "upgrade check" or not, so we can turn it on when we know the upgrade is potential, and then turn it off afterwards.

Of course the other option is to do no checks at all and rely totally on the controller to ramp down and make no requests during the upgrade.

@swcurran your thoughts?

Also regarding the "upgrade check" do we need to check each time we process a received message as well? I.e. if we receive a message and the respective wallet is in the middle of an upgrade should we just leave the message in the queue?

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

I'm going to close this PR and open a new PR to clean it up.